Support NuGet lockfiles by anthony-c-martin · Pull Request #6031 · dependabot/dependabot-core

anthony-c-martin · 2022-11-03T13:11:29Z

First draft at addressing #1303

I'm super unfamiliar with this project as well as Ruby, so guidance is very welcome.

ldeluigi · 2023-01-20T09:39:23Z

Any news on this?

deivid-rodriguez · 2023-01-20T11:09:38Z

Hei @ldeluigi, you just got an update yesterday at #1303. Let's try to not be too pushy please, we get a lot of notifications around here 😅.

ldeluigi · 2023-01-20T11:43:22Z

Hei @ldeluigi, you just got an update yesterday at #1303. Let's try to not be too pushy please, we get a lot of notifications around here 😅.

whops: I'm sorry!

jeffwidman

👋 Sorry for the delay, I've been wanting to get to this and finally had a little time this evening.

A few thoughts:

First, a big caveat, I've never worked with Dotnet/Nuget myself, so any comments may simply be due to misunderstanding.
This looks really solid from a general "dependabot" structure perspective, nice job! 🎉
We split out the dockerfiles by ecosystem, so you'll need to rebase to install dotnet into https://github.com/dependabot/dependabot-core/blob/d0bd472495eda2eabb71040a59dbbfeb4dfb4931/nuget/Dockerfile
I really like that you're leveraging the native tooling... we want to migrate this direction as much as we can.
Today we do all our dotnet updating via ruby/regex text parsing hackery... now that we'd have the native tooling available, does it also support updating the manifest itself? Such that we could start to leverage that to migrate away from our existing text hackery? Obviously something we'd handle in a different PR, but just want to know if we should be creating a new ticket to track opportunities for migration.
CI looks unhappy, mind fixing?

If you can rebase and fix that last CI failure, then I'm 👍 on merging.

I'll also ask a teammate who's more familiar with Nuget to take a look just in case I'm overlooking something, but this looks pretty straightforward so I don't expect any issues. Even if we don't cover all use cases, even adding support for some of them would be a great start for others to build out further.

Thanks again! And sorry took so long to review.

Dockerfile

jeffwidman · 2023-04-08T06:20:17Z

nuget/spec/fixtures/csproj/lockfiles_basic

I'm curious: why does this fixture not have a file extension? No other fixture in this folder is extension-less.

I was also surprised that it's plural "lockfiles" when the content appears to be only a single lockfile?

jeffwidman · 2023-04-19T22:36:39Z

Nudge @anthony-c-martin any chance of addressing my questions?☝️

Be great to move this forward!

jeffwidman · 2023-04-20T00:10:07Z

Note to self: Since this adds lockfile support, we'll need to ensure it supports the lockfile-only strategy, see how other ecosystems are wired up here:

Fix lockfile-only versioning strategy not creating some updates that are expected (v2) #5902

anthony-c-martin · 2023-04-20T15:30:00Z

@jeffwidman - sorry, I've been pretty busy! Will try and take a look over the comments this weekend and sync the branch with main.

Porges · 2023-04-21T04:10:13Z

I'm not entirely sure you want --force-evaluate since that could affect packages other than the one that is trying to update? At least for me it is sufficient to run the basic dotnet restore command to update the lockfile after has updated the .csproj files with the new version.

anthony-c-martin · 2023-04-21T11:24:51Z

I really like that you're leveraging the native tooling... we want to migrate this direction as much as we can.

Today we do all our dotnet updating via ruby/regex text parsing hackery... now that we'd have the native tooling available, does it also support updating the manifest itself? Such that we could start to leverage that to migrate away from our existing text hackery? Obviously something we'd handle in a different PR, but just want to know if we should be creating a new ticket to track opportunities for migration.

These are great points, and something I was wondering about. The dotnet add <package> will take care of both updating the .csproj file, and then updating lockfiles afterwards. There's also dotnet outdated, but it's not built-in tooling, so you may want to understand the implications of taking a dependency.

A caveat that I can think of that needs investigating - You've already got some pretty sophisticated tooling here (for example, you're looking inside Directory.Build.props files for updates). I'm not sure whether the native tools actually support equivalent functionality for some of these edge cases.

ryanbrandenburg · 2023-05-10T18:22:52Z

I'm not sure whether the native tools actually support equivalent functionality for some of these edge cases.

I can confirm that dotnet add package <PackageName> --version <version> does not work in the Directory.Build.props scenario (it replaced Version="$(PackageVersion)" with Version="1.2.3").

neotrow · 2023-06-28T15:09:47Z

is there any news on this? Would love to see depandabot to support nuget lock files :-)

anthony-c-martin · 2023-07-07T18:17:00Z

@jeffwidman sorry for the delay, I've merged and fixed up the CI.

Nishnha · 2023-07-07T22:52:14Z

nuget/lib/dependabot/nuget/file_updater.rb

+          project_file = files.find { |f| project_file?(f) && File.dirname(f.name) == File.dirname(lock_file.name) }
+          next if project_file.nil?


Can you explain this check?

It seems like there can be more than one lockfile (packages.lock.json) per project?

In general there's a lockfile for every project in a solution, and transitive dependencies are represented in each lockfile.

So for example, you could have a structure like the following:

dirs.sln (solution file, contains refs to projA & projB)

projA/projA.csproj (proj file, has nuget reference to nugetX)

projB/projB.csproj (proj file, has project reference to projA)

If dependabot runs for the solution file (dirs.sln), and decides to bump nugetX, you would expect to see:

projA/packages.lock.json has a direct dependency, and is updated

projB/packages.lock.json has a transitive dependency, and is also updated

Here's a concrete example of what this looks like in my project:

Bicep.sln references Bicep.Core & Bicep.Cli

Bicep.Core has a direct dependency on SharpYaml

Bicep.Cli has a project dependency on Bicep.Core (and thus a transitive dependency on SharpYaml)

Both of these lockfiles should be updated if SharpYaml is upgraded:

Bicep.Core

Bicep.Cli

anthony-c-martin · 2023-07-14T01:13:19Z

@jeffwidman, @Nishnha - out of interest, is there any way to manually test this against a repo prior to merging?

Nishnha · 2023-07-25T20:29:18Z

@jeffwidman, @Nishnha - out of interest, is there any way to manually test this against a repo prior to merging?

You could set up a smoke test https://github.com/dependabot/smoke-tests

anthony-c-martin · 2023-09-23T13:46:01Z

@jeffwidman, @Nishnha - out of interest, is there any way to manually test this against a repo prior to merging?

You could set up a smoke test https://github.com/dependabot/smoke-tests

Thanks! I've done this, and temporarily updated this branch to point to my fork, to demonstrate that it's working: https://github.com/dependabot/dependabot-core/actions/runs/6284091213/job/17065146529?pr=6031

milkshakeuk · 2024-01-16T17:27:08Z

@jeffwidman @anthony-c-martin @Nishnha are there any updates on this?

na1307 · 2024-05-05T05:16:45Z

Since the last commit, nuget's dependabot logic has been rewritten in C#. I think this PR should (probably) be updated.

JamieMagee · 2024-05-06T22:13:56Z

Closing this for now, as NuGet support has largely been rewritten in C# and this PR has diverged a lot.

Thank you @anthony-c-martin for the implementation, and thank you @na1307 for picking it up.

anthony-c-martin requested a review from a team as a code owner November 3, 2022 13:11

This was referenced Nov 3, 2022

Support for NuGet package lock files #1303

Closed

Add script to fix dotnet lockfiles Azure/bicep#8954

Merged

jeffwidman force-pushed the lockfiles branch from b9d69f5 to 211f595 Compare January 19, 2023 21:02

abdulapopoola added the L: dotnet:nuget NuGet packages via nuget or dotnet label Mar 30, 2023

jeffwidman reviewed Apr 8, 2023

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

jeffwidman reviewed Apr 8, 2023

View reviewed changes

jeffwidman mentioned this pull request Apr 19, 2023

Add support for Directory.Packages.props file as entrypoint #7086

Merged

NuGet lockfile implementation

fe1d84c

anthony-c-martin force-pushed the lockfiles branch from 211f595 to fe1d84c Compare April 21, 2023 11:08

anthony-c-martin added 4 commits July 7, 2023 13:39

Merge remote-tracking branch 'origin/main' into lockfiles

6eeadf1

Attempt to fix docker build

ca49f1e

Disable dotnet globalization

259ea98

Fix assertion message

463f90f

anthony-c-martin changed the title ~~[POC] NuGet lockfile implementation~~ Support NuGet lockfiles Jul 7, 2023

Nishnha reviewed Jul 7, 2023

View reviewed changes

This was referenced Sep 7, 2023

Update lock files fsprojects/FSharp.Formatting#841

Merged

Directory.Packages.props test for NuGet dependabot/smoke-tests#122

Closed

Merge remote-tracking branch 'origin/main' into lockfiles

5a56a26

anthony-c-martin force-pushed the lockfiles branch 13 times, most recently from e04ad05 to 8fb77ae Compare September 22, 2023 05:34

Test with new smoke test

3e1ac1e

anthony-c-martin force-pushed the lockfiles branch from 8fb77ae to 3e1ac1e Compare September 22, 2023 05:39

Merge remote-tracking branch 'origin/main' into lockfiles

0f77df1

anthony-c-martin force-pushed the lockfiles branch 2 times, most recently from 24a84da to 697aa77 Compare September 23, 2023 13:45

Allow for unchanged lock files

1eb8102

anthony-c-martin force-pushed the lockfiles branch from 697aa77 to 1eb8102 Compare September 23, 2023 13:48

brettfo mentioned this pull request Nov 17, 2023

Contribute changes to NuGet updater from Azure team #8179

Merged

na1307 mentioned this pull request May 6, 2024

Support NuGet lockfiles (Updated) #9678

Merged

JamieMagee closed this May 6, 2024

		project_file = files.find { \|f\| project_file?(f) && File.dirname(f.name) == File.dirname(lock_file.name) }
		next if project_file.nil?

Conversation

anthony-c-martin commented Nov 3, 2022

Uh oh!

ldeluigi commented Jan 20, 2023

Uh oh!

deivid-rodriguez commented Jan 20, 2023

Uh oh!

ldeluigi commented Jan 20, 2023

Uh oh!

jeffwidman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeffwidman Apr 8, 2023

Choose a reason for hiding this comment

Uh oh!

jeffwidman commented Apr 19, 2023

Uh oh!

jeffwidman commented Apr 20, 2023

Uh oh!

anthony-c-martin commented Apr 20, 2023

Uh oh!

Porges commented Apr 21, 2023

Uh oh!

anthony-c-martin commented Apr 21, 2023

Uh oh!

ryanbrandenburg commented May 10, 2023

Uh oh!

neotrow commented Jun 28, 2023

Uh oh!

anthony-c-martin commented Jul 7, 2023

Uh oh!

Nishnha Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

anthony-c-martin Jul 8, 2023

Choose a reason for hiding this comment

Uh oh!

anthony-c-martin commented Jul 14, 2023

Uh oh!

Nishnha commented Jul 25, 2023

Uh oh!

anthony-c-martin commented Sep 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milkshakeuk commented Jan 16, 2024

Uh oh!

na1307 commented May 5, 2024

Uh oh!

JamieMagee commented May 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

anthony-c-martin commented Sep 23, 2023 •

edited

Loading